Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

bullet-featherstone: fix how links are flattened in ConstructSdfModel #562

Merged
merged 7 commits into from
Nov 8, 2023
Merged

bullet-featherstone: fix how links are flattened in ConstructSdfModel #562

merged 7 commits into from
Nov 8, 2023

Conversation

shameekganguly
Copy link
Contributor

Fixes a segfault in ConstructSdfModel when loading a model with links defined out of order. Here's an example model that triggers the segfault:

  <model name="test_model">
      <link name="child_linkC" />
      <link name="parent_link" />
      <link name="child_linkA" />
      <link name="child_linkB" />
      <joint name="world_joint" type="fixed">
        <parent>world</parent>
        <child>parent_link</child>
      </joint>
      <joint name="joint1" type="prismatic">
        <parent>parent_link</parent>
        <child>child_linkA</child>
        <axis>
            <xyz>0 0 1</xyz>
        </axis>
      </joint>
      <joint name="joint2" type="prismatic">
        <parent>child_linkA</parent>
        <child>child_linkB</child>
        <axis>
            <xyz>0 0 1</xyz>
        </axis>
      </joint>
      <joint name="joint3" type="prismatic">
        <parent>child_linkB</parent>
        <child>child_linkC</child>
        <axis>
            <xyz>0 0 1</xyz>
        </axis>
      </joint>
    </model>

The current logic for sorting links to ensure that a parent link is placed in the flatLinks vector before its children links is inadequate for such cases. So the fix is to build the link tree and do a preorder traversal of it to create the flatLinks vector.

@github-actions github-actions bot added the 🌱 garden Ignition Garden label Oct 17, 2023
@codecov
Copy link

codecov bot commented Oct 17, 2023 •
edited
Loading

Codecov Report

Merging #562 (1419fe2) into gz-physics6 (3b7a7c8) will increase coverage by 1.07%.
The diff coverage is 76.19%.

❗ Current head 1419fe2 differs from pull request most recent head ef0ef82. Consider uploading reports for the commit ef0ef82 to get more accurate results

@@               Coverage Diff               @@
##           gz-physics6     #562      +/-   ##
===============================================
+ Coverage        75.95%   77.02%   +1.07%     
===============================================
  Files              143      143              
  Lines             7327     7321       -6     
===============================================
+ Hits              5565     5639      +74     
+ Misses            1762     1682      -80
Files Coverage Δ
bullet-featherstone/src/SDFFeatures.cc 74.15% <76.19%> (+0.09%) ⬆️

... and 2 files with indirect coverage changes

scpeters
scpeters reviewed Oct 23, 2023
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should add a test that uses the snippet you provided. I'll look into the easiest way to do that

@scpeters
Copy link
Member

scpeters commented Nov 2, 2023

we should add a test that uses the snippet you provided. I'll look into the easiest way to do that

I added a test in 49de232 without this branch, and it fails CI. I will push it to this branch

@scpeters
Copy link
Member

scpeters commented Nov 2, 2023

@azeey I added a small test for this in case you want to review

@scpeters scpeters mentioned this pull request Nov 6, 2023
Open
10 tasks
scpeters
scpeters approved these changes Nov 7, 2023
View reviewed changes
Copy link
Member

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, though I think the test I added deserves a second look from someone else if possible

azeey
azeey approved these changes Nov 8, 2023
View reviewed changes
Copy link
Contributor

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Just a few minor comments.

test/common_test/worlds/world_unsorted_links.world Outdated Show resolved Hide resolved
test/common_test/worlds/world_unsorted_links.world Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
test/common_test/world_features.cc Outdated Show resolved Hide resolved
bullet-featherstone/src/SDFFeatures.cc Outdated Show resolved Hide resolved
@scpeters scpeters changed the title Fix how links are flattened in bullet-featherstone ConstructSdfModel bullet-featherstone: fix how links are flattened in ConstructSdfModel Nov 8, 2023
@scpeters scpeters merged commit 1260ef2 into gazebosim:gz-physics6 Nov 8, 2023
11 checks passed
@shameekganguly
Copy link
Contributor Author

Thanks Steve, Ian, Addisu for taking care of this!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@scpeters scpeters scpeters approved these changes

@iche033 iche033 iche033 left review comments

@azeey azeey azeey approved these changes

@mxgrey mxgrey Awaiting requested review from mxgrey

Assignees
No one assigned
Labels
🌱 garden Ignition Garden
Projects
Core development
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@shameekganguly @scpeters @azeey @iche033